Refactor bus wire API to use driver handles#64
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Pyxsim.bus resolved-wire model to use object-oriented “driver handles” bound to wires via a Bus, enabling call sites like controller.sda.release() instead of passing driver identifiers into every operation.
Changes:
- Introduces
Bus,BusDriver, andBusWireDriverto bind drivers to per-wire handles exposed as real attributes (setattr()). - Removes the old string/enum-based
BusWirepublic driver-operation API in favor of internal_drive/_release/_set_driver. - Updates and expands unit tests to cover the new OO binding, uniqueness validation, and attribute exposure behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/python/Pyxsim/bus.py |
Adds OO driver-handle API (Bus, BusDriver, BusWireDriver) and refactors BusWire driver operations into internal methods. |
tests/test_bus_line_model.py |
Migrates tests to OO call sites and adds new tests for binding/validation and per-driver behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
humphrey-xmos
left a comment
There was a problem hiding this comment.
Looks good, yes it does create more objects and more setup code, but hopefully its easier to extend.
| controller.sda.drive(value) | ||
|
|
||
|
|
||
| # New OO API-specific tests |
There was a problem hiding this comment.
Does this also support multiple targets?
There was a problem hiding this comment.
Yes, multiple "Drivers", where a driver can be labels anything you like.
Validate wire names before attribute binding so invalid API input reports clear ValueErrors, ensure repeated binds hit the intended duplicate guard, and make duplicate-name diagnostics deterministic.
Refactor BusWire driver API to use OO driver handles
There was a desire to make the resolved-wire API more object-oriented and easier to read at call sites. Instead of passing a driver identifier into every wire operation, drivers are now registered against wires through a
Bus, which binds each driver to per-wire handles.This adds some setup and indirection internally: the bus topology is declared up front, and each
BusDrivergets real per-wire attributes viasetattr()so call sites can usecontroller.sdadirectly. The underlying wire model still tracks driver state internally, but callers no longer pass string/enum driver names for each operation.Old:
New:
The refactor removes the old string-based driver API, validates wire/driver names during Bus construction, and keeps the existing resolution/clash-detection behavior unchanged.
There are tradeoffs. The new API makes the operation site nicer and more self-describing, but it adds more objects, upfront bus binding, and some internal indirection. To support the controller.sda style without getattr, the implementation uses setattr() to expose bound wire handles as real attributes on each BusDriver. The underlying wire model still tracks per-driver state internally.
One alternative would be index-based access, for example controller[SDA].release() or controller.wire(SDA).release(). That would avoid dynamic attributes and the Python-identifier restriction on wire names, but it is less readable for protocol wires like scl/sda.
So this PR is partly a design tradeoff: if the extra setup and indirection are not worth the cleaner call sites, we may decide to keep the older string/enum-based API instead as it is flatter and easier to understand internally.